RSDK-6574: Fix use of time_point
#335
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lia-viam
requested review from
purplenicole730 and
stuqdog
and removed request for
a team
acmorrow
reviewed
Dec 3, 2024
Member
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
Just interested. I'm happy to see this is being fixed!
| const std::string kRDK = "rdk"; | ||
| const std::string kBuiltin = "builtin"; | ||
|
|
||
| using time_pt = std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>; |
Member
There was a problem hiding this comment.
What motivated the switch from calling it time_point to time_pt?
Collaborator
Author
There was a problem hiding this comment.
I find people do 'using namespace std::chrono' pretty often so I think this would be a name clash with our time_point and std::chrono::time_point<Clock, Duration>
| const std::chrono::nanoseconds nanos(timestamp.nanos()); | ||
| return time_point(std::chrono::duration_cast<std::chrono::system_clock::duration>(seconds) + | ||
| nanos); | ||
| time_pt timestamp_to_time_pt(const google::protobuf::Timestamp& timestamp) { |
Collaborator
Author
There was a problem hiding this comment.
yes I look forward to doing so haha
stuqdog
approved these changes
Dec 3, 2024
Member
stuqdog
left a comment
stuqdog
left a comment
There was a problem hiding this comment.
a question and a nit, otherwise lgtm!
src/viam/sdk/common/utils.hpp
Outdated
| /// std::chrono::time_point<long long, std::chrono::nanoseconds> | ||
| std::chrono::time_point<long long, std::chrono::nanoseconds> timestamp_to_time_pt( | ||
| const google::protobuf::Timestamp& timestamp); | ||
| /// @brief convert a google::protobuf::Timestamp to time_point |
src/viam/sdk/common/utils.hpp
Outdated
| /// a google::protobuf::Timestamp. | ||
| google::protobuf::Timestamp time_pt_to_timestamp( | ||
| const std::chrono::time_point<long long, std::chrono::nanoseconds>& time_pt); | ||
| /// @brief convert a time_point to a google::protobuf::Timestamp. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the incorrect
std::chrono::time_pointtype used throughout the SDK. I have opted to call thisviam::sdk::time_ptso as not to create name clashes withtime_pointfor anyone doingusing namespace std::chrono.